Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grantmaster M1 #1057

Merged
merged 4 commits into from
Jan 2, 2024
Merged

Grantmaster M1 #1057

merged 4 commits into from
Jan 2, 2024

Conversation

Zaniyar
Copy link
Contributor

@Zaniyar Zaniyar commented Nov 13, 2023

Milestone Delivery Checklist

  • The milestone-delivery-template.md has been copied and updated.
  • This pull request is being made by the same account as the accepted application.
  • I have disclosed any and all sources of reused code in the submitted repositories and have done my due diligence to meet its license requirements.
  • In case of acceptance, an invoice must be submitted and the payment will be transferred to the Polkadot/fiat account provided in the application.
  • The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#1765

@semuelle semuelle self-assigned this Nov 20, 2023
@semuelle
Copy link
Member

Hey @Zaniyar. Thanks for the delivery! I am trying to run the unit tests locally, but they are failing with the following error message. Any idea what might be wrong?

    SyntaxError: Cannot use import statement outside a module

      1 | import { ArrowLeftOutlined, CheckOutlined, CloseOutlined, SyncOutlined } from '@ant-design/icons';
      2 | import { Button, Card, Descriptions, Tabs } from 'antd';
    > 3 | import axios from 'axios';
        | ^
      4 | import { useEffect, useState } from 'react';
      5 | import { useNavigate, useParams } from 'react-router-dom';
      6 | import ReactMarkdown from 'react-markdown';

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1728:14)
      at Object.require (src/components/PullRequestDetail.tsx:3:1)
      at Object.require (src/App.tsx:2:1)
      at Object.require (src/App.test.tsx:3:1)
      at TestScheduler.scheduleTests (node_modules/@jest/core/build/TestScheduler.js:333:13)
      at runJest (node_modules/@jest/core/build/runJest.js:404:19)

@Zaniyar
Copy link
Contributor Author

Zaniyar commented Nov 22, 2023

@semuelle sorry about that, I didn't have the issue at first on my machine, but after cloning the project freshly, I was able to reproduce it. I decided to remove the legacy frontend test that didn't really do anything and I added more tests on the parser side. Can you check again?

@semuelle
Copy link
Member

Thanks for the update, @Zaniyar. I can confirm that the tests are working now. I have another issue, though. When I run the system through docker-compose, the frontend will show the following error. Any suggestions?

ERROR in src/App.test.tsx:8:23
TS2339: Property 'toBeInTheDocument' does not exist on type 'JestMatchers<HTMLElement>'.
     6 |   render(<App />);
     7 |   const linkElement = screen.getByText(/learn react/i);
  >  8 |   expect(linkElement).toBeInTheDocument();
       |                       ^^^^^^^^^^^^^^^^^
     9 | });
    10 |

@Zaniyar
Copy link
Contributor Author

Zaniyar commented Nov 26, 2023

@semuelle sorry about that, I forgot to commit some changes. Now it should work - could you try again?

@semuelle
Copy link
Member

Hi @Zaniyar. I am one step further, thanks. However, the crawler doesn't seem to read the values from the .env file (at least when running though Docker), because I am getting a 401 AxisError with https://api.github.com/repos/undefined/undefined/pulls as the URL. I have tried rebuilding the api image with no success. Could you look into that?

@Zaniyar
Copy link
Contributor Author

Zaniyar commented Nov 30, 2023

@semuelle sorry about that. I've got the same problem on a fresh copy, this didn't happen on my working copy. Also, the values of the env variables don't need to be quoted, I removed them from the example file. I pushed the fix, which I've tested on macOs and ubuntu - it should work now. I just noticed that you could hit the rate limit of githubs API, when syncing all historical PRs. I'm already working on a fix for that.

Copy link
Member

@semuelle semuelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Using docker-compose, it's working now. When I try to run each image manually, I get a ECONNREFUSED on the crawler. I also found a process variable (process.env.MONGO_HOST) that I didn't see defined anywhere and tried changing that as well, but no success. Not sure what I'm missing there, but it's probably just my setting.

The Level, Total FTE and milestones seem to be missing from all applications. Is this not implemented, or is there an issue with the parser?

image

Also, just a sidenote, but "changes requested" doesn't mean that the reviewer has rejected the application.

@semuelle
Copy link
Member

pinging @Zaniyar

@semuelle
Copy link
Member

Hey @Zaniyar, any updates on this?

@Zaniyar
Copy link
Contributor Author

Zaniyar commented Dec 20, 2023

@semuelle My day job kept me busy with year-end related activities, but I'm still on it. While I've already fixed the issues related to the level, FTE and payment address parsing, I'm still working on fixing the milestones and the github rate limit issues that occur when crawling the historical PRs.

Regarding the ECONNREFUSED message, I wasn't able to reproduce it; does the error occur when running the npm commands manually?

Regarding the rate limit issues, I've come up with 2 different strategies to resolve it. Please let me know, which one you'd prefer and I'll implement it accordingly.

i) solution 1: Set the github rate limit with an environment variable. Whenever that limit is reached, the crawler will stop and wait for 1h (since rate limits on github seem to be set on an 1h basis). This will allow to set the limit to a lower number in order to completely avoid running into limits.
ii) solution 2: Don't set an application-level limit and allow for the limit to be reached. Once the limit is reached, no more github requests are possible for 1h. If we choose this path, I suggest reflecting the "rate limit reached" state on the ui (e.g. by using a label, colored in red).

In both solutions, whenever the historical PRs (i.e. closed & merged) are crawled, the PRs that are alraeady present in the DB will be ignored to avoid pulling the same data twice. Note that this doesn't apply to crawling open PRs.

@semuelle
Copy link
Member

Hey @Zaniyar. I imagine that we wouldn't encounter the rate limit issue much. Once the historic PRs are crawled, there is not that much activity to crawl day to day. So feel free to submit the changes you have made to parse the level, FTE, etc. and I'll have another look.

Regarding the ECONNREFUSED issue: what's the value of MONGO_HOST supposed to be? What credentials are you using to connect to it?

image

@Zaniyar
Copy link
Contributor Author

Zaniyar commented Dec 27, 2023

@semuelle First of all, happy holidays.🎉 I've now pushed the fixes for the FTE, payment address, level as well as milestone parser logic. The bugs were mainly related to incorrect assumptions regarding the actual contents of the applications, rather than the template itself. I updated the tests along with the fixes.

Regarding the MONGO_HOST variable, this was a legacy problem that has been taken care of. I'm not sure why it worked for me, despite of having the wrong environment variable in place. But anyhow, the only mongo-related env variable that is needed is MONGODB_URI. Could you double-check if you've instantiated that one correctly in the ".env" file?

  • if you run api & frontend locally, it should be set to "MONGODB_URI=mongodb://localhost:27017/grants-dashboard"
  • if you run api & frontend using docker, it should be set to "MONGODB_URI=mongodb://mongo:27017/grants-dashboard"

Copy link
Member

@semuelle semuelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, @Zaniyar. Your milestone is hereby accepted. I had one more issue where the frontend didn't catch an issue with the Github token (Error fetching PR IDs from GitHub: AxiosError: Request failed with status code 401) and instead showed a success message, but everything else is working as expected. You can find the evaluation notes here.

@semuelle semuelle merged commit b069b79 into w3f:master Jan 2, 2024
3 checks passed
Copy link

github-actions bot commented Jan 2, 2024

🪙 Please fill out the invoice form in order to initiate the payment process. Thank you!

@RouvenP
Copy link

RouvenP commented May 31, 2024

hi @Zaniyar we sent the payment this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants